Add MySQL integration tests (8.0, 8.4, 9.1)#22
Conversation
Tests single sandbox and replication topology deployment against real MySQL binaries. Runs on push to master, PRs, nightly, and manual dispatch. MySQL tarballs are cached between runs.
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes add a new integration test GitHub Actions workflow for MySQL deployments, update Linux CI dependencies, restrict linter configuration to core checks, add error handling in documentation utilities, and implement conditional test skipping based on sandbox binary availability. Changes
Sequence Diagram(s)sequenceDiagram
actor GitHub as GitHub Actions
participant Workflow as Integration Test Workflow
participant Build as Build dbdeployer
participant Cache as Cache Layer
participant MySQL as MySQL Tarball
participant Sandbox as Sandbox System
participant Tests as Test Execution
GitHub->>Workflow: Trigger (push/PR/cron/manual)
Workflow->>Build: Setup Go 1.22 & checkout repo
Build->>Build: go build -o dbdeployer .
Workflow->>Cache: Check cached MySQL tarball
alt Cache Hit
Cache-->>MySQL: Return cached tarball
else Cache Miss
Workflow->>MySQL: Download version-specific tarball
MySQL-->>Cache: Store for future use
end
Build->>Sandbox: ./dbdeployer unpack (populate binary dir)
par Single Instance Tests
Sandbox->>Tests: Deploy single sandbox
Tests->>Tests: SELECT VERSION() validation
Tests->>Sandbox: Delete all sandboxes
and Replication Tests
Sandbox->>Tests: Deploy replication sandbox
Tests->>Tests: check_slaves, test_replication
Tests->>Sandbox: Delete all sandboxes
end
Workflow->>Sandbox: dbdeployer delete all (cleanup)
Workflow->>Workflow: Force-kill leftover mysqld processes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated GitHub Actions workflow to run integration tests for dbdeployer against real MySQL server binaries across multiple MySQL versions.
Changes:
- Introduces a new
Integration Testsworkflow running on pushes/PRs tomaster, nightly, and manual dispatch. - Runs a matrix over MySQL
8.0.42,8.4.4, and9.1.0, deploying and validating both single and replication sandboxes. - Adds caching for downloaded MySQL tarballs and an always-run cleanup step.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SHORT_VER=$(echo "$MYSQL_VERSION" | grep -oP '^\d+\.\d+') | ||
| TARBALL="mysql-${MYSQL_VERSION}-linux-glibc2.17-x86_64.tar.xz" | ||
| mkdir -p /tmp/mysql-tarball | ||
| if [ ! -f "/tmp/mysql-tarball/$TARBALL" ]; then | ||
| echo "Downloading $TARBALL..." | ||
| curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ | ||
| "https://dev.mysql.com/get/Downloads/MySQL-${SHORT_VER}/$TARBALL" \ | ||
| || curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ | ||
| "https://downloads.mysql.com/archives/get/p/23/file/$TARBALL" | ||
| fi | ||
| ls -lh "/tmp/mysql-tarball/$TARBALL" | ||
|
|
||
| - name: Unpack MySQL | ||
| run: | | ||
| mkdir -p "$SANDBOX_BINARY" | ||
| TARBALL="mysql-${MYSQL_VERSION}-linux-glibc2.17-x86_64.tar.xz" | ||
| ./dbdeployer unpack "/tmp/mysql-tarball/$TARBALL" \ |
There was a problem hiding this comment.
The workflow downloads MySQL tarballs via raw curl URLs, which bypasses dbdeployer’s existing downloads machinery (user-agent handling for dev.mysql.com, checksum verification when available, retries, etc.). This increases supply-chain risk and makes the workflow more brittle if MySQL changes download behavior. Consider replacing the custom download logic with dbdeployer downloads get-by-version / get-unpack (run from /tmp/mysql-tarball so caching still works), or at minimum add pinned SHA256 verification for each version in the matrix.
| SHORT_VER=$(echo "$MYSQL_VERSION" | grep -oP '^\d+\.\d+') | |
| TARBALL="mysql-${MYSQL_VERSION}-linux-glibc2.17-x86_64.tar.xz" | |
| mkdir -p /tmp/mysql-tarball | |
| if [ ! -f "/tmp/mysql-tarball/$TARBALL" ]; then | |
| echo "Downloading $TARBALL..." | |
| curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ | |
| "https://dev.mysql.com/get/Downloads/MySQL-${SHORT_VER}/$TARBALL" \ | |
| || curl -L -f -o "/tmp/mysql-tarball/$TARBALL" \ | |
| "https://downloads.mysql.com/archives/get/p/23/file/$TARBALL" | |
| fi | |
| ls -lh "/tmp/mysql-tarball/$TARBALL" | |
| - name: Unpack MySQL | |
| run: | | |
| mkdir -p "$SANDBOX_BINARY" | |
| TARBALL="mysql-${MYSQL_VERSION}-linux-glibc2.17-x86_64.tar.xz" | |
| ./dbdeployer unpack "/tmp/mysql-tarball/$TARBALL" \ | |
| mkdir -p /tmp/mysql-tarball | |
| cd /tmp/mysql-tarball | |
| "$GITHUB_WORKSPACE/dbdeployer" downloads get-by-version "$MYSQL_VERSION" | |
| ls -lh | |
| - name: Unpack MySQL | |
| run: | | |
| mkdir -p "$SANDBOX_BINARY" | |
| cd /tmp/mysql-tarball | |
| "$GITHUB_WORKSPACE/dbdeployer" downloads get-unpack "$MYSQL_VERSION" \ |
| - name: Test single sandbox | ||
| run: | | ||
| ./dbdeployer deploy single "$MYSQL_VERSION" \ | ||
| --sandbox-binary="$SANDBOX_BINARY" | ||
| ~/sandboxes/msb_*/use -e "SELECT VERSION()" | ||
| ./dbdeployer delete all --skip-confirm | ||
|
|
||
| - name: Test replication sandbox | ||
| run: | | ||
| ./dbdeployer deploy replication "$MYSQL_VERSION" \ | ||
| --sandbox-binary="$SANDBOX_BINARY" | ||
| ~/sandboxes/rsandbox_*/check_slaves | ||
| ~/sandboxes/rsandbox_*/test_replication | ||
| ./dbdeployer delete all --skip-confirm |
There was a problem hiding this comment.
These steps rely on ~/sandboxes/msb_* / ~/sandboxes/rsandbox_* globbing and then call dbdeployer delete all, which could unintentionally target unrelated sandboxes if anything else created them in the runner’s home directory. To make the job self-contained and deterministic, set SANDBOX_HOME to a job-local directory (e.g., inside ${{ github.workspace }}) and reference $SANDBOX_HOME/... explicitly; then delete only within that directory (or delete the specific sandbox name you just created).
| if: always() | ||
| run: | | ||
| ./dbdeployer delete all --skip-confirm 2>/dev/null || true | ||
| pkill -9 mysqld 2>/dev/null || true |
There was a problem hiding this comment.
pkill -9 mysqld is very broad and can terminate any mysqld process on the runner, not just the ones started by this job. If extra cleanup is needed beyond dbdeployer delete, consider scoping the kill to the current user and/or sandbox home (e.g., by reading PID files under the sandbox directory) to avoid collateral termination.
| pkill -9 mysqld 2>/dev/null || true | |
| pkill -9 -u "$USER" mysqld 2>/dev/null || true |
…nings - libncurses5 → libncurses6, libaio1 → libaio1t64 (ubuntu-latest is now 24.04) - Suppress G115 gosec warnings (safe int64→uint64 casts on file sizes) - Suppress errcheck on lockfile.Unlock (standard defer pattern)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/integration_tests.yml (1)
70-83: Quote glob patterns to prevent unexpected behavior.The glob patterns
~/sandboxes/msb_*/useand~/sandboxes/rsandbox_*/...trigger SC2211. While these work when exactly one sandbox exists, they could fail unexpectedly if the glob doesn't match (literal pattern executed) or matches multiple directories.♻️ Proposed fix using bash glob safety
- name: Test single sandbox run: | ./dbdeployer deploy single "$MYSQL_VERSION" \ --sandbox-binary="$SANDBOX_BINARY" - ~/sandboxes/msb_*/use -e "SELECT VERSION()" + for sb in ~/sandboxes/msb_*/; do "$sb/use" -e "SELECT VERSION()"; done ./dbdeployer delete all --skip-confirm - name: Test replication sandbox run: | ./dbdeployer deploy replication "$MYSQL_VERSION" \ --sandbox-binary="$SANDBOX_BINARY" - ~/sandboxes/rsandbox_*/check_slaves - ~/sandboxes/rsandbox_*/test_replication + for sb in ~/sandboxes/rsandbox_*/; do "$sb/check_slaves"; done + for sb in ~/sandboxes/rsandbox_*/; do "$sb/test_replication"; done ./dbdeployer delete all --skip-confirm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration_tests.yml around lines 70 - 83, The shell step uses unguarded globs like ~/sandboxes/msb_*/use and ~/sandboxes/rsandbox_*/check_slaves which can expand to zero or multiple paths; enable bash glob safety (shopt -s nullglob) and explicitly expand each glob into an array, validate the array length (fail if zero or >1) and then invoke the single element (e.g., msb=(~/sandboxes/msb_*/use); use "${msb[0]}"), and do the same for the rsandbox_* patterns before running check_slaves/test_replication and before cleanup to ensure deterministic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 16-19: The global G112 suppression and the broad errcheck unlock
suppression are too wide; update .golangci.yml to restrict G112 to test files
only and remove the global errcheck exclusion for lock.Unlock, then fix the
specific occurrence in defaults/catalog.go by either using defer lock.Unlock(),
handling the returned error explicitly instead of "_ = lock.Unlock()", or add a
file-scoped errcheck exclusion comment directly above the intentional unlock if
you must ignore it; reference G112, errcheck, lock.Unlock, and
defaults/catalog.go to locate and apply these changes.
- Around line 23-26: Remove the global errcheck exclusion for "lock.Unlock" from
.golangci.yml and update all call sites to handle Unlock errors: in
ReadCatalog() wrap the deferred Unlock in a closure that captures unlockErr and,
if the function uses a named error return (err), set err = unlockErr when err is
nil; in UpdateCatalog() and DeleteFromCatalog() wrap the deferred Unlock in a
closure that checks unlockErr and either logs it (using the package logger) or
returns it according to those functions' signatures; ensure every use of
lock.Unlock() (the three functions ReadCatalog, UpdateCatalog,
DeleteFromCatalog) properly checks and handles the returned error instead of
ignoring it.
---
Nitpick comments:
In @.github/workflows/integration_tests.yml:
- Around line 70-83: The shell step uses unguarded globs like
~/sandboxes/msb_*/use and ~/sandboxes/rsandbox_*/check_slaves which can expand
to zero or multiple paths; enable bash glob safety (shopt -s nullglob) and
explicitly expand each glob into an array, validate the array length (fail if
zero or >1) and then invoke the single element (e.g.,
msb=(~/sandboxes/msb_*/use); use "${msb[0]}"), and do the same for the
rsandbox_* patterns before running check_slaves/test_replication and before
cleanup to ensure deterministic behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a24e4e3-25e5-489b-affc-f548d2228a3d
📒 Files selected for processing (6)
.github/workflows/all_tests.yml.github/workflows/integration_tests.yml.golangci.ymlmkreadme/make_readme.gomkwiki/make_docs.gotest/go-unit-tests.sh
| gosec: | ||
| excludes: | ||
| - G115 # integer overflow int64->uint64 on file sizes — safe | ||
| - G112 # Slowloris in test code only |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether G112-relevant server code exists outside tests.
rg -nP --type=go -g '!**/*_test.go' 'http\.Server\s*\{' -C2
rg -nP --type=go -g '!**/*_test.go' '\bReadHeaderTimeout\s*:' -C1Repository: ProxySQL/dbdeployer
Length of output: 45
🏁 Script executed:
# Verify .golangci.yml exists and check the exact state around lines 16-19
cat -n .golangci.yml | head -30Repository: ProxySQL/dbdeployer
Length of output: 686
🏁 Script executed:
# Broader search for HTTP server usage in non-test code
rg -i 'http\.Server|net\.Listen|ListenAndServe' --type=go -g '!**/*_test.go' | head -20Repository: ProxySQL/dbdeployer
Length of output: 45
🏁 Script executed:
# Check for any Unlock() usage in codebase
rg 'Unlock\s*\(' --type=go -B2 -A2 | head -40Repository: ProxySQL/dbdeployer
Length of output: 1143
🏁 Script executed:
# Understand the repository structure and purpose
ls -la && cat README.md | head -50Repository: ProxySQL/dbdeployer
Length of output: 6783
Scope G112 suppression to test files as general practice; lock.Unlock suppression requires attention.
Line 19 disables G112 globally, which is unnecessary for this codebase: a search for HTTP servers in non-test code returns no results (dbdeployer is a CLI deployment tool, not an HTTP server). While scoping to test files is still good lint hygiene, the original justification (masking server code) does not apply here.
However, the broader concern about line 24–26 (lock.Unlock suppression) is valid. The global exclusion suppresses all errcheck findings for unlock operations. While most code properly uses defer lock.Unlock(), defaults/catalog.go shows an intentional _ = lock.Unlock() that bypasses the check. Rather than suppress errors globally, fix the specific unlock error at its source or add a scoped exclusion if intentional.
Suggested config adjustment
linters-settings:
errcheck:
check-type-assertions: false
gosec:
excludes:
- G115 # integer overflow int64->uint64 on file sizes — safe
- - G112 # Slowloris in test code only
issues:
exclude-use-default: true
exclude-rules:
+ - path: '.*_test\.go'
+ linters:
+ - gosec
+ text: "G112"
- linters:
- errcheck
text: "lock.Unlock"Alternatively, if lock.Unlock errors are intentional, add context via a scoped rule rather than suppressing all occurrences.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 16 - 19, The global G112 suppression and the
broad errcheck unlock suppression are too wide; update .golangci.yml to restrict
G112 to test files only and remove the global errcheck exclusion for
lock.Unlock, then fix the specific occurrence in defaults/catalog.go by either
using defer lock.Unlock(), handling the returned error explicitly instead of "_
= lock.Unlock()", or add a file-scoped errcheck exclusion comment directly above
the intentional unlock if you must ignore it; reference G112, errcheck,
lock.Unlock, and defaults/catalog.go to locate and apply these changes.
| exclude-rules: | ||
| - linters: | ||
| - errcheck | ||
| text: "lock.Unlock" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find unchecked Unlock() calls that may be hidden by errcheck exclusions.
rg -nP --type=go -C2 '\b_?\s*=\s*[A-Za-z_][A-Za-z0-9_]*\.Unlock\s*\(\)|\bdefer\s+[A-Za-z_][A-Za-z0-9_]*\.Unlock\s*\(\)'Repository: ProxySQL/dbdeployer
Length of output: 638
🏁 Script executed:
# Check the .golangci.yml to confirm the exclude-rule
cat .golangci.ymlRepository: ProxySQL/dbdeployer
Length of output: 504
🏁 Script executed:
# Get broader context around the Unlock calls in defaults/catalog.go
cat -n defaults/catalog.go | sed -n '65,80p;140,150p;165,180p'Repository: ProxySQL/dbdeployer
Length of output: 1185
🏁 Script executed:
# Search for all lock.Unlock calls to see complete picture
rg -n "lock\.Unlock" --type=goRepository: ProxySQL/dbdeployer
Length of output: 194
🏁 Script executed:
# Get function names for the other two Unlock calls
cat -n defaults/catalog.go | sed -n '135,150p;160,180p'Repository: ProxySQL/dbdeployer
Length of output: 1072
🏁 Script executed:
# Find the function name for the Unlock at line 145
cat -n defaults/catalog.go | sed -n '120,145p'Repository: ProxySQL/dbdeployer
Length of output: 902
Remove the errcheck exclusion for lock.Unlock and handle errors at all call sites.
The global suppress rule in .golangci.yml masks unchecked unlock errors across three functions:
ReadCatalog()(line 73):_ = lock.Unlock()UpdateCatalog()(line 145):defer lock.Unlock()DeleteFromCatalog()(line 171):defer lock.Unlock()
Remove the exclusion and apply proper error handling to all three functions:
Suggested fix
issues:
exclude-use-default: true
- exclude-rules:
- - linters:
- - errcheck
- text: "lock.Unlock"
+ exclude-rules: []// For ReadCatalog()
defer func() {
if unlockErr := lock.Unlock(); unlockErr != nil && err == nil {
err = unlockErr
}
}()
// For UpdateCatalog() and DeleteFromCatalog()
defer func() {
if unlockErr := lock.Unlock(); unlockErr != nil {
// Log or return the error based on function signature
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exclude-rules: | |
| - linters: | |
| - errcheck | |
| text: "lock.Unlock" | |
| exclude-rules: [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 23 - 26, Remove the global errcheck exclusion for
"lock.Unlock" from .golangci.yml and update all call sites to handle Unlock
errors: in ReadCatalog() wrap the deferred Unlock in a closure that captures
unlockErr and, if the function uses a named error return (err), set err =
unlockErr when err is nil; in UpdateCatalog() and DeleteFromCatalog() wrap the
deferred Unlock in a closure that checks unlockErr and either logs it (using the
package logger) or returns it according to those functions' signatures; ensure
every use of lock.Unlock() (the three functions ReadCatalog, UpdateCatalog,
DeleteFromCatalog) properly checks and handles the returned error instead of
ignoring it.
807165e to
7553a03
Compare
Summary
Adds a GitHub Actions workflow that tests dbdeployer against real MySQL binaries.
What it tests
MySQL versions tested
When it runs
Features
fail-fast: false— all versions tested even if one failsTest plan
Summary by CodeRabbit
Tests
Chores